Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Always exit unsuccessfully with sys.exit #1481

Merged
merged 1 commit into from
Oct 5, 2024
Merged

Always exit unsuccessfully with sys.exit #1481

merged 1 commit into from
Oct 5, 2024

Conversation

chrisnovakovic
Copy link
Contributor

Description

exit isn't defined as a function or imported in main.py, so reaching any code path that attempts to execute exit(1) results in a NameError being raised:

Traceback (most recent call last):
    File "/usr/lib/python3.10/runpy.py", line 196, in _run_module_as_main
    return _run_code(code, main_globals, None,
    File "/usr/lib/python3.10/runpy.py", line 86, in _run_code
    exec(code, run_globals)
    File "[...]/__main__.py", line 415, in <module>
    File "[...]/__main__.py", line 399, in run
    File "[...]/__main__.py", line 389, in main
    File "/usr/lib/python3.10/runpy.py", line 220, in run_module
    mod_name, mod_spec, code = _get_module_details(mod_name)
    File "/usr/lib/python3.10/runpy.py", line 157, in _get_module_details
    code = loader.get_code(mod_name)
    File "[...]/__main__.py", line 264, in get_code
    File "[...]/__main__.py", line 204, in load_module
    File "/usr/lib/python3.10/importlib/__init__.py", line 126, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
    File "<frozen importlib._bootstrap>", line 1050, in _gcd_import
    File "<frozen importlib._bootstrap>", line 1027, in _find_and_load
    File "<frozen importlib._bootstrap>", line 1006, in _find_and_load_unlocked
    File "<frozen importlib._bootstrap>", line 688, in _load_unlocked
    File "<frozen importlib._bootstrap_external>", line 883, in exec_module
    File "<frozen importlib._bootstrap>", line 241, in _call_with_frames_removed
    File "third_party/python3/__pgcli_main__.py", line 3, in <module>
    File "[...]/click/core.py", line 1157, in __call__
    File "[...]/click/core.py", line 1078, in main
    File "[...]/click/core.py", line 1434, in invoke
    File "[...]/click/core.py", line 783, in invoke
    File "[...]/pgcli/main.py", line 1617, in cli
    File "[...]/pgcli/main.py", line 766, in connect
NameError: name 'exit' is not defined. Did you mean: 'atexit'?

When exiting unsuccessfully, do so with sys.exit(1) - this is already used in several other places in main.py, and is always used (with exit code 0) when exiting successfully.

Checklist

  • I've added this contribution to the changelog.rst.
  • I've added my name to the AUTHORS file (or it's already there).
  • I installed pre-commit hooks (pip install pre-commit && pre-commit install), and ran black on my code.
  • Please squash merge this pull request (uncheck if you'd like us to merge as multiple commits)

@dbaty
Copy link
Member

dbaty commented Sep 26, 2024 •
edited
Loading

exit is "usually" available, see https://docs.python.org/3/library/constants.html#exit. In fact, I cannot reproduce this error when triggerin code that calls exit :

$ pgcli --dsn non_existent_dsn
Could not find a DSN with alias non_existent_dsn. Please check the "[alias_dsn]" section in pgclirc.
$

How did you install "pgcli" and how do you run it such that you have this error?

I don't disagree with your proposed changes, I am just curious! :)

@chrisnovakovic
Copy link
Contributor Author

This happens when running pgcli via the Please build system (more specifically when pgcli is defined as a python_wheel target via the python-rules plugin). python_wheel packages Python modules and all of their transitive dependencies into a single zip file with some bootstrap code prepended so importlib knows where to find everything.

To make sure there are no conflicts between modules installed system-wide and the packaged versions, the interpreter is invoked with -S (i.e., disabling the site module that would normally provide the exit function), so you can probably reproduce this outside of Please by running python3 -S -m pgcli .... I'll update my commit message to make the source of the failure clearer.

@chrisnovakovic
Always exit unsuccessfully with sys.exit
1ed8528 
`exit` is undefined when the `site` module isn't loaded (i.e. when
Python is executed with the `-S` option), and `exit` isn't explicitly
defined as a function or imported from elsewhere in `main.py`, so
reaching any code path that attempts to execute `exit(1)` in this
environment results in a `NameError` being raised:

```
Traceback (most recent call last):
    File "/usr/lib/python3.10/runpy.py", line 196, in _run_module_as_main
    return _run_code(code, main_globals, None,
    File "/usr/lib/python3.10/runpy.py", line 86, in _run_code
    exec(code, run_globals)
    File "[...]/__main__.py", line 415, in <module>
    File "[...]/__main__.py", line 399, in run
    File "[...]/__main__.py", line 389, in main
    File "/usr/lib/python3.10/runpy.py", line 220, in run_module
    mod_name, mod_spec, code = _get_module_details(mod_name)
    File "/usr/lib/python3.10/runpy.py", line 157, in _get_module_details
    code = loader.get_code(mod_name)
    File "[...]/__main__.py", line 264, in get_code
    File "[...]/__main__.py", line 204, in load_module
    File "/usr/lib/python3.10/importlib/__init__.py", line 126, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
    File "<frozen importlib._bootstrap>", line 1050, in _gcd_import
    File "<frozen importlib._bootstrap>", line 1027, in _find_and_load
    File "<frozen importlib._bootstrap>", line 1006, in _find_and_load_unlocked
    File "<frozen importlib._bootstrap>", line 688, in _load_unlocked
    File "<frozen importlib._bootstrap_external>", line 883, in exec_module
    File "<frozen importlib._bootstrap>", line 241, in _call_with_frames_removed
    File "third_party/python3/__pgcli_main__.py", line 3, in <module>
    File "[...]/click/core.py", line 1157, in __call__
    File "[...]/click/core.py", line 1078, in main
    File "[...]/click/core.py", line 1434, in invoke
    File "[...]/click/core.py", line 783, in invoke
    File "[...]/pgcli/main.py", line 1617, in cli
    File "[...]/pgcli/main.py", line 766, in connect
NameError: name 'exit' is not defined. Did you mean: 'atexit'?
```

When exiting unsuccessfully, do so with `sys.exit(1)` - this is already
used in several other places in `main.py`, and is always used (with exit
code 0) when exiting successfully.
@j-bennet
Copy link
Contributor

j-bennet commented Oct 5, 2024 •
edited
Loading

Apparently sys.exit is recommended for use in program code. I suggest merging if tests pass, @dbaty . Thank you!

@j-bennet j-bennet merged commit 9b6925e into dbcli:main Oct 5, 2024
7 checks passed
@j-bennet
Copy link
Contributor

j-bennet commented Oct 5, 2024

@chrisnovakovic Thank you for the PR! 🍬

@chrisnovakovic chrisnovakovic deleted the main-exit-1 branch October 5, 2024 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@chrisnovakovic @dbaty @j-bennet